drivers: pinctrl: wch_20x_30x_afio: fix afio remap #87397
Conversation
| @@ -33,6 +33,10 @@ int pinctrl_configure_pins(const pinctrl_soc_pin_t *pins, uint8_t pin_cnt, uintp | |||
| uint32_t pcfr = pcfr_id == 0 ? AFIO->PCFR1 : AFIO->PCFR2; | |||
| uint8_t cfg = 0; | |||
|
|
|||
| if (remap != 0) { | |||
| RCC->APB2PCENR |= RCC_AFIOEN; | |||
There was a problem hiding this comment.
The AFIO clock is enabled after reading AFIO->PCFR1 or AFIO->PCFR2 (line 33).
Shouldn't it be enabled before that?
There was a problem hiding this comment.
You're right. I've revised the code. Please review again.
recalci
force-pushed
the
fix_ch32v2_remap
branch
from
83f22e0 to
98904cd
Compare
There was a problem hiding this comment.
I'm still not okay with writing into AFIO->PCFRx when remap is disabled.
I think the whole alternate function handling can be made more compact.
There is no reason to have part of it at the beginning and the rest at the end.
Also I'm positive that handling of CH32V20X_V30X_PINMUX_USART1_RM is not quite correct.
Let's consider remap 3 of USART1: bit 0 is located in PCFR1 and bit 1 in PCFR2 but the code will set both bits in PCFR1 (lines 80 and 83) and additionally bit 1 in PCFR2 (line 92).
The additional bit 1 written into PCFR1 will change USART2 remapping.
E.g: pinctrl_wch_20x_30x_afio.c
if (remap != 0) {
RCC->APB2PCENR |= RCC_AFIOE;
if (bit0 == CH32V20X_V30X_PINMUX_USART1_RM) {
AFIO->PCFR1 |= ((uint32_t)((remap >> 0) & 1)
<< (CH32V20X_V30X_PINMUX_USART1_RM & 0x1F));
AFIO->PCFR2 |= ((uint32_t)((remap >> 1) & 1)
<< (CH32V20X_V30X_PINMUX_USART1_RM1 & 0x1F));
} else {
if (pcfr_id == 0) {
AFIO->PCFR1 |= remap << bit0;
} else {
AFIO->PCFR2 |= remap << bit0;
}
}
}
drivers/pinctrl/pinctrl_wch_afio.c also has writes into disabled AFIO peripheral but pcfr1 handling seems to be okay.
There was a problem hiding this comment.
One more point to consider:
if (bit0 == CH32V20X_V30X_PINMUX_USART1_RM) {
The condition above matches not only USART1_RM but also TIM8_RM.
CH32V20X_V30X_PINMUX_USART1_RM is 2, CH32V20X_V30X_PINMUX_TIM8_RM is (32+2) but 32 is not part of bit0 but becomes pcfr_id instead.
So bit0 is 2 for both CH32V20X_V30X_PINMUX_USART1_RM and CH32V20X_V30X_PINMUX_TIM8_RM.
For correct matching we also need to consider pcfr_id.
E.g.:
if (pcfr_id == 0 && bit0 == CH32V20X_V30X_PINMUX_USART1_RM) {
98904cd to
7d93f4b
Compare
- Enable AFIO clock prior to remap configuration - Consolidate remap logic in a single conditional block - Correct USART1 remap detection by checking pcfr_id - Apply changes to pinctrl_wch_afio.c Signed-off-by: Jianxiong Gu <jianxiong.gu@outlook.com>
recalci
force-pushed
the
fix_ch32v2_remap
branch
from
7d93f4b to
d865a9c
Compare
|
@msalau It's much clearer. Updated as your comments. Thank you! |
|
FYI, #89138 enables the AFIO clocks in a different way. I think it's mildly better as the clock ID is defined in Devicetree, and it works for EXTI remaps. |
Enable the AFIO clock before remap. Otherwise, remap will not work.
Same as #83353